What's wrong with the moving?

It's impossible to piece together the peripheral's state if the user drops the transaction, especially in async.

What's wrong with the moving?

It's impossible to piece together the peripheral's state if the user drops the transaction, especially in async.

Wasn't the State enum doing that? Or are thinking about some other state.

Btw how did you know setting the datalen to zero would get the SPI to stop? Trial and error? Or was this a hidden nugget of info in esp-idf?

Btw how did you know setting the datalen to zero would get the SPI to stop?

Completely by accident. As far as I can see, esp-idf has no concept of aborting a queued (or other in-progress) transfer.

The questions around the async API will be discussed during today's Rust Embedded meeting: rust-embedded/wg#796 (comment)

Just did a quick code size test and there is a 13K difference in my app (for ESP32-S3) between this branch and the branch's base. I'll happily take a 0.9% code size difference from just the SPI driver.

App/part. size:    1,502,816/2,031,616 bytes, 73.97% 4422ed3c
App/part. size:    1,489,536/2,031,616 bytes, 73.32% 1423f84

What would it take to convince you to keep the moving in SpiDmaBus?

I'm not opposed to creating short-lived buffer types that may end up getting moved (and I'm even taking a step in that direction in a later PR), but I'd rather not move the peripheral itself internally, and I think the current state demonstrates that at least that part isn't necessary.

and I think the current state demonstrates that at least that part isn't necessary.

It never was.
The motivation for moving stuff in the bus was to make sure there was only one SPI API, the one that moves stuff. (Idk if you've followed #1512 and the issues/PRs that spawn from it.)
Any other SPI niceties would be built on top of this move API, and in such a manner that users could copy it and do it themselves with any necessary customization to the wrapper. Stuff like queuing (like esp-idf does it) and double buffering.
Another nice thing from using the move API in SpiDmaBus, was that the SpiDmaBus didn't use unsafe and we could rely on the Rust compiler to check the invariants. HALs (this one and esp-idf-hal) have a tendency to sprinkle unsafe everywhere internally. imo the unsafe should be kept at the lowest level and everything else should be build on top of that without the use of unsafe, but I'm probably the only one who feels this way.

Having said all that, you can disregard that comment/request from me. Adding the new dma buffer view stuff to SPI has been a challenge (due to the full duplex nature of the peripheral), I probably won't be touching the SPI driver until all the other drivers use the dma buffers.

The motivation for moving stuff in the bus was to make sure there was only one SPI API, the one that moves stuff. (Idk if you've followed #1512 and the issues/PRs that spawn from it.)

This PR doesn't change that public API, but allows us some more flexibility in our impl, whilst also solving cancellation safety (something that I guess is going to be a problem using the move API in down stream crates, but I don't think we need to worry about that just yet).

Another nice thing from using the move API in SpiDmaBus, was that the SpiDmaBus didn't use unsafe and we could rely on the Rust compiler to check the invariants. HALs (this one and esp-idf-hal) have a tendency to sprinkle unsafe everywhere internally. imo the unsafe should be kept at the lowest level and everything else should be build on top of that without the use of unsafe, but I'm probably the only one who feels this way.

I do tend to agree with your principle in general, but I don't think we're using unsafe for the sake of it, we're doing it for a reason.

Tbh if AsyncDrop existed life would be a whole lot easier - but until then IMO this is a fine trade-off, plus the size reduction is a nice additional benefit.